Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add gameboy memory callback values #3904

Draft
wants to merge 40 commits into
base: master
Choose a base branch
from

Conversation

roydmerkel
Copy link
Contributor

@roydmerkel roydmerkel commented Apr 28, 2024

  • Fixed != in MemoryDomain.
  • Exposed GB bank registers in SGB BSNES, SGB Libsnes, Gambette, Sameboy, GBHawk
  • Exposed values written in callbacks for Gambette, Sameboy, and GBHawk

Check if completed:

merged master into branch
bsnes build broke when rewind.c was removed because it defined the GB_rewind_reset, which was being linked in despite being no longer present. -DGB_DISABLE_REWIND turns off this link.
…moved bank registers for sameboy to another file.
@CasualPokePlayer
Copy link
Member

CasualPokePlayer commented Apr 28, 2024

Some immediate issues I'm seeing.

  1. The submodule locations should not change. If any changes are needed, they should occur upstream (not an issue for gambatte, since we control it effectively).
  2. On that, why did you change SameBoy's submodule? The only change is something with upstream's Makefile which we don't use at all anyways, our provided CMakeLists is how we build SameBoy.
  3. Gambatte already exposes bank registers, as part of IDebuggable's GetCpuFlagsAndRegisters. You've seemed to have "exposed" it now as some public function not associated with any interface (more or less not useful for lua and even for ApiHawk that's not particularly good).
  4. Don't bother touching the old BSNES core, it's legacy and subject to be removed eventually once new BSNES has its final nits resolved.

@roydmerkel
Copy link
Contributor Author

Some immediate issues I'm seeing.

  1. The submodule locations should not change. If any changes are needed, they should occur upstream (not an issue for gambatte, since we control it effectively).
  2. On that, why did you change SameBoy's submodule? The only change is something with upstream's Makefile which we don't use at all anyways, the CMakeLists is how we build SameBoy.
  3. Gambatte already exposes bank registers, as part of IDebuggable's GetCpuFlagsAndRegisters. You've seemed to have "exposed" it now as some public function not associated with any interface (more or less not useful for lua and even for ApiHawk that's not particularly good).
  4. Don't bother touching the old BSNES core, it's legacy and subject to be removed eventually once new BSNES has its final nits resolved.

on 1: yeah ,that's an oversight on my end, looks like sameboy change has already been merged, but I haven't updated the submodule path, I'll submit a change for that. For Gambette, it's still sitting in PR awaiting approval.
2. see 1. I was having build issues and it's already merged, I'll create an update commit to update the new upstream.
3. let me play arround with that one, since I put the bank registers back into the original function to reduce changes, I may not need the other function.
4. ok, I'll revert those changes.

…ginally split off for speed reasons and because bsnes wasn't updating correctly when in registers, however, this looks unneccessary by testing.)
@roydmerkel
Copy link
Contributor Author

@CasualPokePlayer , addressed 1-4, I am tempted to revert libsnes changes as well as memory and execution breakpoints cause exceptions in libsnes for "SGB System Bus", and other domains don't seem to do anything.

I'll probably do this then ping you in channel in Discord for re-review. But let me know if there's anything in my libsnes changes that you think would be useful to keep, otherwise, I'll probably revert the libsnes changes.

@roydmerkel
Copy link
Contributor Author

ok, rolled back libsnes changes as callbacks aren't supported on supergameboy memory in libsnes or bsnes, so the change was pointless, so I went ahead and rolled it back as it was one of your action items.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants